Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Fix Notebook entry hover problem #7106

Merged
merged 13 commits into from
Dec 14, 2023
Merged

Fix Notebook entry hover problem #7106

merged 13 commits into from
Dec 14, 2023

Conversation

charlesh88
Copy link
Contributor

@charlesh88 charlesh88 commented Oct 3, 2023 •
edited
Loading

Closes #7105

  • Removed :not(:focus) CSS check for hover.
  • New theme constant for a more subdued hover effect to differentiate from active editing mode.

Closes

Describe your changes:

CSS was not being triggered on hover when the field was the focus - this was an artifact of a previous workflow when we were using contenteditable on a div. That check was removed, and the hover styling has been refined.

All Submissions:

  • Have you followed the guidelines in our Contributing document?
  • Have you checked to ensure there aren't other open Pull Requests for the same update/change?
  • Is this change backwards compatible? For example, developers won't need to change how they are calling the API or how they've extended core plugins such as Tables or Plots.

Author Checklist

  • Changes address original issue?
  • Tests included and/or updated with changes?
  • Command line build passes?
  • Has this been smoke tested?
  • Testing instructions included in associated issue OR is this a dependency/testcase change?

Reviewer Checklist

  • Changes appear to address issue?
  • Reviewer has tested changes by following the provided instructions?
  • Changes appear not to be breaking changes?
  • Appropriate automated tests included?
  • Code style and in-line documentation are appropriate?
  • Has associated issue been labelled unverified? (only applicable if this PR closes the issue)
  • Has associated issue been labelled bug? (only applicable if this PR is for a bug fix)

@charlesh88
Closes #7105
4bf68d5 
- Removed `:not(:focus)` CSS check for hover.
- New theme constant for a more subdued hover effect to differentiate
from active editing mode.
@deploysentinel
Copy link

deploysentinel bot commented Oct 3, 2023 •
edited
Loading

Current Playwright Test Results Summary

✅ 165 Passing - ⚠️ 4 Flaky

Run may still be in progress, this comment will be updated as current testing workflow or job completes...

(Last updated on 12/14/2023 02:38:49pm UTC)

Run Details

Running Job e2e-stable on CircleCI

Commit: 3fa8e07

Started: 12/14/2023 02:31:02pm UTC

⚠️ Flakes

📄   functional/plugins/tabs/tabs.e2e.spec.js • 1 Flake

Test Case Results

Test Case Last 7 days Failures Last 7 days Flakes
Tabs View Renders tabbed elements
Retry 1 • Initial Attempt
0% (0) 0 / 35 runs
failed over last 7 days
17.14% (6) 6 / 35 runs
flaked over last 7 days
📄   functional/plugins/telemetryTable/telemetryTable.e2e.spec.js • 1 Flake

Test Case Results

Test Case Last 7 days Failures Last 7 days Flakes
Telemetry Table unpauses and filters data when paused by button and user changes bounds
Retry 1 • Initial Attempt
0% (0) 0 / 36 runs
failed over last 7 days
16.67% (6) 6 / 36 runs
flaked over last 7 days
📄   functional/planning/timelist.e2e.spec.js • 1 Flake

Test Case Results

Test Case Last 7 days Failures Last 7 days Flakes
Time List Create a Time List, add a single Plan to it and verify all the activities are displayed with no milliseconds
Retry 1 • Initial Attempt
6.67% (4) 4 / 60 runs
failed over last 7 days
60% (36) 36 / 60 runs
flaked over last 7 days
📄   functional/plugins/notebook/restrictedNotebook.e2e.spec.js • 1 Flake

Test Case Results

Test Case Last 7 days Failures Last 7 days Flakes
Restricted Notebook with a page locked and with an embed @addinit Allows embeds to be deleted if page unlocked @addinit
Retry 1 • Initial Attempt
0% (0) 0 / 36 runs
failed over last 7 days
41.67% (15) 15 / 36 runs
flaked over last 7 days

View Detailed Build Results

scottbell
scottbell approved these changes Oct 3, 2023
View reviewed changes
Copy link
Contributor

@scottbell scottbell left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good stuff @charlesh88

Screen.Recording.2023-10-03.at.9.19.12.AM.mov

@ozyx ozyx added this to the Target:3.2.0 milestone Oct 3, 2023
@codecov
Copy link

codecov bot commented Oct 10, 2023 •
edited
Loading

Codecov Report

Merging #7106 (f6d9161) into master (250db8d) will increase coverage by 0.30%.
The diff coverage is n/a.

❗ Current head f6d9161 differs from pull request most recent head 3fa8e07. Consider uploading reports for the commit 3fa8e07 to get more accurate results

Additional details and impacted files 
@@            Coverage Diff             @@
##           master    #7106      +/-   ##
==========================================
+ Coverage   55.66%   55.97%   +0.30%     
==========================================
  Files         655      654       -1     
  Lines       26316    26229      -87     
  Branches     2543     2528      -15     
==========================================
+ Hits        14650    14681      +31     
+ Misses      10961    10843     -118     
  Partials      705      705
Flag Coverage Δ *Carryforward flag
e2e-full 41.91% <ø> (+0.04%) ⬆️ Carriedforward from 2dc1388
e2e-stable 58.31% <ø> (-0.15%) ⬇️
unit 49.25% <ø> (+0.47%) ⬆️

*This pull request uses carry forward flags. Click here to find out more.

see 35 files with indirect coverage changes

Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 250db8d...3fa8e07. Read the comment docs.

@scottbell scottbell added the pr:e2e:couchdb npm run test:e2e:couchdb label Oct 17, 2023
@github-actions github-actions bot removed the pr:e2e:couchdb npm run test:e2e:couchdb label Oct 17, 2023
@charlesh88 charlesh88 marked this pull request as ready for review October 24, 2023 17:38
@charlesh88 charlesh88 enabled auto-merge (squash) October 24, 2023 22:15
@ozyx ozyx disabled auto-merge November 2, 2023 21:39
ozyx
ozyx previously requested changes Nov 2, 2023
View reviewed changes
Copy link
Contributor

@ozyx ozyx left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's add a quick visual test

@charlesh88 charlesh88 enabled auto-merge (squash) November 7, 2023 21:56
@unlikelyzero unlikelyzero modified the milestones: Target:3.2.1, Target:3.3.0 Nov 13, 2023
unlikelyzero
unlikelyzero approved these changes Dec 14, 2023
View reviewed changes
Copy link
Collaborator

@unlikelyzero unlikelyzero left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ozyx @charlesh88 test looks great!

@charlesh88 charlesh88 merged commit 97bffc5 into master Dec 14, 2023
14 of 15 checks passed
@charlesh88 charlesh88 deleted the fix-entry-hover-7105 branch December 14, 2023 14:39
@unlikelyzero unlikelyzero modified the milestones: Target:3.3.0, Target:4.0.0 Feb 1, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@unlikelyzero unlikelyzero unlikelyzero approved these changes

@scottbell scottbell scottbell approved these changes

@ozyx ozyx Awaiting requested review from ozyx

Assignees
No one assigned
Labels
type:bug
Projects
None yet
Milestone
Target:4.0.0
Development

Successfully merging this pull request may close these issues.

[Notebook] Notebook entry hover isn't triggering properly
4 participants
@charlesh88 @ozyx @scottbell @unlikelyzero